Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Best practices for regular expressions versus Regex performance review #1348

Merged
merged 20 commits into from
Nov 16, 2024

Conversation

EngRajabi
Copy link
Contributor

@EngRajabi EngRajabi commented Oct 6, 2020

Proposed changes

Regex improvements:

  • set option compile
  • set timeout
  • set static object (Regex class is immutable)

Links

Hossein-Edraki
Hossein-Edraki previously approved these changes Oct 7, 2020
Copy link

@Hossein-Edraki Hossein-Edraki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's Ok

@EngRajabi
Copy link
Contributor Author

@TomPallister

VahidTajari
VahidTajari previously approved these changes Nov 15, 2020
@WeihanLi
Copy link
Contributor

What if timeout happens? Timeout maybe not good practice here.

@EngRajabi
Copy link
Contributor Author

Setting timeout is one of the necessary tasks of using regex. When a timeout occurs, it is best to run the entire project involved and not respond to others.

@WeihanLi
Copy link
Contributor

Is there some reason for considering 100ms as the regex timeout, and how could the client update the timeout config if the client wanna use a longer timeout?

@EngRajabi
Copy link
Contributor Author

timeout is not customizable by the customer. Like many other settings. It was 100 ms when I adjusted it according to the tests I had. But we can decide here and change this time. We can set this time to 500 ms. If we do not receive an answer at this time, it is better not to involve the whole project in its implementation and let it respond to other requests.

@EngRajabi EngRajabi dismissed stale reviews from VahidTajari and Hossein-Edraki via 9ba1128 February 12, 2021 16:13
@raman-m raman-m changed the base branch from master to develop July 17, 2023 12:23
@raman-m raman-m changed the title Improvement regex Regex improvements Jul 17, 2023
@raman-m raman-m self-requested a review July 17, 2023 12:58
Copy link
Contributor Author

@EngRajabi EngRajabi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review done

@raman-m
Copy link
Member

raman-m commented Jul 20, 2023

Dear Mohsen,

Why did you merge develop branch from your fork into the feature branch?
I meant these commits: ff1dbb6 5b336cd
It was unnecessary because I did the merge of ThreeMammals:develop in 329d2c3.
After that I trigger pipeline to make a valid green build for commit 03a70b3.

Now there is no build at all and pipeline cannot run new build.
The pipeline cannot recognize develop branch because you have juggled all commits in a wrong way.

Could you fix/update your fork develop branch please first?

After that we will try to fix pipeline triggering by rebasing current feature branch onto develop one.

Copy link
Contributor Author

@EngRajabi EngRajabi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did Branch Develop on Branch as Rebase. Does this solve this problem?

@raman-m
Copy link
Member

raman-m commented Jul 22, 2023

image

Now I see develop and it has zero diff, but the branch is not default. ☝️
You have to go to repo settings and make develop branch as default!
I hope that should help to fix the problem of builds, because all builds are built for develop branch (auto) and master (manually).
Currently I don't see that pipeline was triggered for top commit.
Maybe an empty or tiny commit will be required to trigger the pipeline after making develop branch default.

Copy link
Contributor Author

@EngRajabi EngRajabi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change something for build

@raman-m raman-m force-pushed the improvement_regex branch from bfdecf7 to 4b9516c Compare July 22, 2023 13:58
@raman-m
Copy link
Member

raman-m commented Jul 22, 2023

Feature branch has been rebased onto develop!
The build has been passed for top commit!

Going to start code review...

I guess, this PR is not related to an issue, right?

@raman-m raman-m removed the conflicts Feature branch has merge conflicts label Nov 7, 2024
@raman-m
Copy link
Member

raman-m commented Nov 7, 2024

@EngRajabi Hello, Mohsen! Are you energized and ready to deliver this PR? 😄

@raman-m raman-m changed the title Regex improvements Best practices for regular expressions aka Regex improvements Nov 7, 2024
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for Delivery ✅

@raman-m raman-m changed the title Best practices for regular expressions aka Regex improvements Best practices for regular expressions versus Regex performance review Nov 16, 2024
@raman-m raman-m added the Winter'25 Winter 2025 release label Nov 16, 2024
@raman-m raman-m added this to the Autumn'24 milestone Nov 16, 2024
@raman-m raman-m merged commit d76fc95 into ThreeMammals:develop Nov 16, 2024
1 check passed
@ThreeMammals ThreeMammals deleted a comment from ggnaegi Nov 16, 2024
@ggnaegi
Copy link
Member

ggnaegi commented Nov 16, 2024

@raman @RaynaldM Again, I would have prefered moving to .NET 7-9 before to reduce the complexity and ensure a better maintainability.

@EngRajabi EngRajabi deleted the improvement_regex branch November 17, 2024 04:03
@raman-m raman-m added Nov'24 November 2024 release and removed Winter'25 Winter 2025 release labels Nov 20, 2024
@raman-m raman-m modified the milestones: Autumn'24, November'24 Nov 20, 2024
@raman-m raman-m mentioned this pull request Dec 4, 2024
5 tasks
@raman-m
Copy link
Member

raman-m commented Dec 13, 2024

@ggnaegi commented Nov 16, 2024

Please update to the desired .NET version in your forked repository, or remain on version 23.4.2, which corresponds .NET 7.
To minimize complexity and ensure better maintainability, we adhere to the standard Microsoft .NET support lifecycle, specifically the .NET and .NET Core Support Policy.NET 7End of support on May 14, 2024❗
Our official end of .NET 6, 7 support is merged PRs #2220 #2221 → version 23.4.2

P.S. Ocelot is an open-source project, just like .NET framework. You would be correct if Ocelot were a commercial project, but it is not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Ocelot Core related or system upgrade (not a public feature) Nov'24 November 2024 release proposal Proposal for a new functionality in Ocelot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants